[keyvault-keys] Browser support, with record and playback #4343

sadasant · 2019-07-17T21:22:48Z

Everything should be working at this point! I'll review myself, clean this and follow up with another PR for keyvault-secrets.

Related to #3797

sadasant · 2019-07-17T21:31:11Z

sdk/keyvault/keyvault-keys/package.json

-    "lint:fix": "eslint -c ../../.eslintrc.json src tests samples --ext .ts --fix --fix-type [problem,suggestion]",
-    "lint": "eslint -c ../../.eslintrc.json src tests samples --ext .ts -f node_modules/eslint-detailed-reporter/lib/detailed.js -o keyvault-keys-lintReport.html || exit 0",
+    "lint:fix": "eslint --rule 'no-invalid-this: 0' -c ../../.eslintrc.json src tests samples --ext .ts --fix --fix-type [problem,suggestion]",
+    "lint": "eslint --rule 'no-invalid-this: 0' -c ../../.eslintrc.json src tests samples --ext .ts -f node_modules/eslint-detailed-reporter/lib/detailed.js -o keyvault-keys-lintReport.html || exit 0",


Added this rule since I'm using this.title and other properties of Mocha's test prototypes in my tests.

I don't like having special lint rules for specific packages. Why is this rule specific to this package? Should the test code for this package instead be modified to align with other packages which don't need this special rule?

sadasant · 2019-07-17T21:43:09Z

sdk/keyvault/keyvault-keys/tests/utils/recorder.ts

@@ -161,6 +179,190 @@ class NockRecorder extends Recorder {
  }
 }

+class NiseRecorder extends Recorder {


This is taken as is from storage-queue.

...rowsers/keys_client__create_read_update_and_delete_operations/recording_before_all_hook.json

HarshaNalluru · 2019-07-18T00:22:02Z

sdk/keyvault/keyvault-keys/karma.conf.js

+    customLaunchers: {
+      ChromeHeadlessNoSandbox: {
+        base: "ChromeHeadless",
+        flags: ["--no-sandbox", "--disable-web-security"]


Why is this flag --no-sandbox required ?

I haven't been able to make it work in my Linux machine without this.

Is this the same situation for storage-queue for you?
I don't recall facing any such issues with storage packages when I executed the tests on my Ubuntu sometime before.

This is necessary for me on all libraries that use chrome headless 😅 I believe @daviwil knows an alternative, but I believe it changes the kernel or something.

sdk/keyvault/keyvault-keys/package.json

sdk/keyvault/keyvault-keys/.eslintignore

sdk/keyvault/keyvault-keys/api-extractor.json

sdk/keyvault/keyvault-keys/gulpfile.js

sdk/keyvault/keyvault-keys/package.json

HarshaNalluru · 2019-07-18T00:32:05Z

sdk/keyvault/keyvault-keys/tsconfig.json

-  "exclude": ["node_modules", "types/**", "./samples/**/*.ts", "./test/perf/azure-sb-package/*.ts"],
-  "include": ["./src/**/*.ts", "./test/**/*.ts"]
+  "exclude": ["node_modules", "./samples/**/*.ts"],
+  "include": ["./src/**/*.ts", "./tests/**/*.ts"]


[NIT]
For consistency, name of the tests folder 📂 tests -> test
servicebus, template and storage packages follow the name "test".

This is true! The only caveat is that this PR size will likely increase considerably. Can I change the name of the folder in another PR?

mikeharder · 2019-07-18T00:35:54Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2019-07-18T00:36:03Z

Azure Pipelines successfully started running 1 pipeline(s).

mikeharder · 2019-07-18T00:37:33Z

@sadasant: Please do not merge until we get a passing run of pipeline "js - keyvault-keys - tests" (which I manually triggered above).

sadasant · 2019-07-18T20:14:47Z

sdk/keyvault/keyvault-keys/tests/utils/recorder.ts

+  }
+
+  public stop(): void {
+    for (let i = 0; i < this.recordings.length; i++) {


I had to add this for loop to properly filter secret values in this recorder!

sadasant · 2019-07-18T20:53:13Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2019-07-18T20:53:24Z

Azure Pipelines successfully started running 1 pipeline(s).

sadasant · 2019-07-19T00:01:24Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2019-07-19T00:01:33Z

Azure Pipelines successfully started running 1 pipeline(s).

sadasant · 2019-07-19T00:10:46Z

sdk/keyvault/keyvault-keys/review/keyvault-keys.api.md

@@ -4,6 +4,7 @@

 ```ts

+import { AbortSignalLike } from '@azure/abort-controller';


I merged a test with abortSignal and I didn't spot the missing definition in the API. I'll address this in all of the other methods in another Pull Request. I made this issue to follow up on this: #4357

sdk/keyvault/keyvault-keys/.eslintignore

mikeharder · 2019-07-19T17:23:37Z

sdk/keyvault/keyvault-keys/package.json

    "tsconfig.json"
  ],
+  "browser": {
+    "./dist/index.js": "./browser/azure-keyvault-keys.min.js",
+    "./dist-esm/src/index.js": "./dist-esm/src/index.js",


I think this line is a no-op and should be removed. There's no point to map a file to itself.

mikeharder · 2019-07-19T17:25:17Z

sdk/keyvault/keyvault-keys/package.json

-    "lint:fix": "eslint -c ../../.eslintrc.json src tests samples --ext .ts --fix --fix-type [problem,suggestion]",
-    "lint": "eslint -c ../../.eslintrc.json src tests samples --ext .ts -f node_modules/eslint-detailed-reporter/lib/detailed.js -o keyvault-keys-lintReport.html || exit 0",
+    "lint:fix": "eslint --rule 'no-invalid-this: 0' -c ../../.eslintrc.json src tests samples --ext .ts --fix --fix-type [problem,suggestion]",
+    "lint": "eslint --rule 'no-invalid-this: 0' -c ../../.eslintrc.json src tests samples --ext .ts -f node_modules/eslint-detailed-reporter/lib/detailed.js -o keyvault-keys-lintReport.html || exit 0",


I don't like having special lint rules for specific packages. Why is this rule specific to this package? Should the test code for this package instead be modified to align with other packages which don't need this special rule?

sdk/keyvault/keyvault-keys/package.json

mikeharder

Approved pending comment explaining eslint exclusion and successful live test pass. If you've already seen live tests pass and no significant changes were made since then, you don't need to wait for tests again.

sdk/keyvault/keyvault-keys/.eslintignore

…ignore

sadasant · 2019-07-19T18:34:10Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2019-07-19T18:34:20Z

Azure Pipelines successfully started running 1 pipeline(s).

Everything should be working at this point! This is the same work done for keyvault-keys here: #4343 Related to #3797

sadasant added Client This issue points to a problem in the data-plane of the library. KeyVault labels Jul 17, 2019

sadasant requested review from daviwil, sophiajt, mikeharder and HarshaNalluru July 17, 2019 21:22

sadasant self-assigned this Jul 17, 2019

sadasant commented Jul 17, 2019

View reviewed changes

HarshaNalluru reviewed Jul 17, 2019

View reviewed changes

...rowsers/keys_client__create_read_update_and_delete_operations/recording_before_all_hook.json Outdated Show resolved Hide resolved

HarshaNalluru reviewed Jul 18, 2019

View reviewed changes

sdk/keyvault/keyvault-keys/package.json Outdated Show resolved Hide resolved

HarshaNalluru reviewed Jul 18, 2019

View reviewed changes

sdk/keyvault/keyvault-keys/package.json Outdated Show resolved Hide resolved

mikeharder requested changes Jul 18, 2019

View reviewed changes

HarshaNalluru reviewed Jul 18, 2019

View reviewed changes

sadasant force-pushed the feature/keyVaultBrowser branch from 5f81d3f to e190f77 Compare July 18, 2019 20:12

sadasant commented Jul 18, 2019

View reviewed changes

sadasant mentioned this pull request Jul 18, 2019

[keyvault-secrets] Browser support, with record and playback #4354

Merged

sadasant added 7 commits July 18, 2019 21:13

wip

cb6c704

wip

82395c6

test authentication

0038d86

updated on top of the latest master

b67f2c1

terser and externalNodeBuiltins

96134e8

node tests are working

6dd1db6

adding karma

df08e73

sadasant added 8 commits July 18, 2019 21:13

trying to make this work, but failing

e94d697

a bit more tidy, but no visible improvement

5adbaf3

wip

3d809c8

fixing identity

dd4eeff

We might not need ClientSecretCredential

04c67d8

tests are working + addressed recent feedback

7ecc3ad

fixing the abort issue after rebasing over latest master

e157936

fixes after rebase

1bf81e6

sadasant force-pushed the feature/keyVaultBrowser branch from e190f77 to 1bf81e6 Compare July 18, 2019 23:58

sadasant commented Jul 19, 2019

View reviewed changes

prettier

72cfc94

mikeharder requested changes Jul 19, 2019

View reviewed changes

sadasant added 2 commits July 19, 2019 17:48

latest feedback addressed

dc6855b

unnecessary self-referential path

e29c5af

mikeharder approved these changes Jul 19, 2019

View reviewed changes

sdk/keyvault/keyvault-keys/.eslintignore Show resolved Hide resolved

added comment explaining why we're ignoring the core files in .eslint…

b538ee3

…ignore

sadasant merged commit 5bb8353 into Azure:master Jul 19, 2019

sadasant deleted the feature/keyVaultBrowser branch July 19, 2019 20:26

sadasant added a commit that referenced this pull request Jul 19, 2019

[keyvault-secrets] Browser support, with record and playback (#4354)

f27498e

Everything should be working at this point! This is the same work done for keyvault-keys here: #4343 Related to #3797

sadasant mentioned this pull request Jul 22, 2019

[JS] [KeyVault] Switch from ts-mocha to mocha #4193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[keyvault-keys] Browser support, with record and playback #4343

[keyvault-keys] Browser support, with record and playback #4343

sadasant commented Jul 17, 2019 •

edited

Loading

sadasant Jul 17, 2019

mikeharder Jul 19, 2019

sadasant Jul 19, 2019

sadasant Jul 17, 2019

HarshaNalluru Jul 18, 2019

sadasant Jul 18, 2019 •

edited

Loading

HarshaNalluru Jul 18, 2019 •

edited

Loading

sadasant Jul 18, 2019

HarshaNalluru Jul 18, 2019

sadasant Jul 18, 2019

mikeharder commented Jul 18, 2019

azure-pipelines bot commented Jul 18, 2019

mikeharder commented Jul 18, 2019

sadasant Jul 18, 2019

sadasant commented Jul 18, 2019

azure-pipelines bot commented Jul 18, 2019

sadasant commented Jul 19, 2019

azure-pipelines bot commented Jul 19, 2019

sadasant Jul 19, 2019

mikeharder Jul 19, 2019

mikeharder Jul 19, 2019

mikeharder left a comment

sadasant commented Jul 19, 2019

azure-pipelines bot commented Jul 19, 2019

		@@ -4,6 +4,7 @@

		```ts

		import { AbortSignalLike } from '@azure/abort-controller';

[keyvault-keys] Browser support, with record and playback #4343

[keyvault-keys] Browser support, with record and playback #4343

Conversation

sadasant commented Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

HarshaNalluru Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharder commented Jul 18, 2019

azure-pipelines bot commented Jul 18, 2019

mikeharder commented Jul 18, 2019

Choose a reason for hiding this comment

sadasant commented Jul 18, 2019

azure-pipelines bot commented Jul 18, 2019

sadasant commented Jul 19, 2019

azure-pipelines bot commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharder left a comment

Choose a reason for hiding this comment

sadasant commented Jul 19, 2019

azure-pipelines bot commented Jul 19, 2019

sadasant commented Jul 17, 2019 •

edited

Loading

sadasant Jul 18, 2019 •

edited

Loading

HarshaNalluru Jul 18, 2019 •

edited

Loading